feat(kernel): mTLS client-certificate option + ABI-version check#409
feat(kernel): mTLS client-certificate option + ABI-version check#409mani-mathur-arch wants to merge 18 commits into
Conversation
Wire the kernel backend's two blocking cgo calls to the new cancellable C-ABI entry points via a ctxWatcher that bridges a Go context onto a kernel cancel token: - OpenSession -> kernel_session_open_cancellable - rows.go nextBatch -> kernel_result_stream_next_batch_cancellable Previously each blocked in an uninterruptible cgo call: a slow warehouse cold-start or a hung CloudFetch chunk ignored the caller's ctx deadline. The watcher fires the token on ctx.Done(), which drops the in-flight kernel request future (a real abort), and the call sites prefer the ctx error on cancellation while preserving the kernel error as cause (matching the execute path and the database/sql convention). A session-fatal error is evicted before the ctx-cancelled return so a failure racing a cancel still evicts the conn. A non-cancellable ctx (nil Done) yields a nil watcher -> NULL token -> the plain, unchanged path, so there is zero watcher overhead on the common case. Requires the kernel cancel-token symbols; bump KERNEL_REV to the merged kernel revision before this builds in CI (it links a locally-staged archive today). Tests: tagged ctxWatcher unit tests (fires on cancel/deadline, nil-safe on an uncancellable ctx, clean teardown without fire) exercising the real cgo ctx->token bridge in the build-and-test-kernel CI job. Default CGO_ENABLED=0 build unchanged. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Bumps the kernel pin from the placeholder statement-surface rev to the tip of the tier2-features branch (databricks-sql-kernel#167 @ 02c0a43), the head of the unmerged kernel PR stack (#165 -> #166 cancel-token -> #167 tier2). It's a linear superset carrying every new C-ABI symbol these driver branches link against: kernel_session_open_cancellable / kernel_result_stream_next_batch_cancellable (cancel token, #166) plus kernel_abi_version, kernel_session_close_blocking, set_tls_client_certificate, set_cloudfetch_enabled (#167). The kernel-lib build fetches the bare commit via its PR-head-ref fallback, so an unmerged SHA is buildable. Temporary: re-pin to the squash-merge SHA once the kernel stack lands. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
42af196 to
26cf09b
Compare
…etch cancel Address PR #409 review findings: - OpenSession's cancelled-connect path now wraps BOTH the ctx error and the underlying kernel error (two %w), so errors.Is still matches the ctx error while the *KernelError stays reachable via errors.As. Mirrors the execute (operation.go) and next_batch (rows.go) cancelled paths, which already did this; the connect path was dropping the kernel diagnostics. - Add TestKernelE2ECancelDuringFetch, which deterministically reaches rows.Next() after QueryContext succeeds and then observes cancellation during fetch — closing the coverage gap where the read-path cancel could pass without exercising kernel_result_stream_next_batch_cancellable.
…etch cancel Address PR #409 review findings: - OpenSession's cancelled-connect path now wraps BOTH the ctx error and the underlying kernel error (two %w), so errors.Is still matches the ctx error while the *KernelError stays reachable via errors.As. Mirrors the execute (operation.go) and next_batch (rows.go) cancelled paths, which already did this; the connect path was dropping the kernel diagnostics. - Add TestKernelE2ECancelDuringFetch, which deterministically reaches rows.Next() after QueryContext succeeds and then observes cancellation during fetch — closing the coverage gap where the read-path cancel could pass without exercising kernel_result_stream_next_batch_cancellable. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
662aedc to
7c094f8
Compare
The previous TestKernelE2ECancelDuringFetch cancelled between batches, so nextBatch's pre-fetch ctx.Err() guard short-circuited before ever calling kernel_result_stream_next_batch_cancellable — it only proved the read path honored ctx, not the in-flight token abort. Rework it to cancel while a fetch is actually blocked: a watcher goroutine fires the cancel only once a single rows.Next() has been blocked longer than any buffered-row return could take (i.e. it is inside the network fetch). The run is verified to reach the cancellable call by the "next_batch cancelled" wrapper the C-call error path emits (the pre-fetch guard returns a bare context error); attempts are retried a bounded number of times to absorb the timing window. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…ions
Wire the four new kernel C-ABI symbols on the driver side, extending the
existing experimental-config surface:
- checkABIVersion(): a sync.Once handshake at the top of OpenSession compares
the linked library's kernel_abi_version() against the header's
DATABRICKS_KERNEL_ABI_VERSION and refuses to connect on a mismatch, so a
differently-built prebuilt .a can't be silently misread.
- WithKernelClientCertificate(cert, key): mTLS client identity forwarded via
the paired kernel_session_config_set_tls_client_certificate. Both PEM halves
are required together — validateKernelConfig rejects an unpaired credential
loudly so a lone key can't be silently dropped — and the key is never logged.
- WithKernelCloudFetch(enabled): a tri-state *bool (nil keeps the kernel
default on; set forwards kernel_session_config_set_cloudfetch_enabled).
Distinct from the plain-bool WithCloudFetch, whose unset state can't be told
from false.
All three are kernel-only and rejected loudly on the Thrift path (the connector
fails when KernelExperimental is non-nil). The reflective classification guard is
extended so a new experimental field can't ship unforwarded/unrejected.
CloseSession stays fire-and-forget: the C ABI now also offers
kernel_session_close_blocking, but adopting it would make close a blocking
network round-trip with no deadline honored, so swapping to it is grouped with
the cancellable-close follow-up.
Requires the kernel ABI-version / mTLS / CloudFetch symbols; bump KERNEL_REV to
the merged kernel revision before this builds in CI (it links a locally-staged
archive today).
Tests: experimental-field classification guard + option wiring + Thrift
rejection + DeepCopy (untagged); TestSetKernelTLS mTLS case, TestABIVersionMatches,
and mTLS-pairing validation (tagged / config). Default CGO_ENABLED=0 build
unchanged.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
WithKernelClientCertificate marks the kernel experimental config as set, but an empty cert+key pair (e.g. from a failed PEM load) was indistinguishable from the option never being called: the old XOR validation accepted it and applyKernelTLS skipped the setter, so a caller who explicitly requested mTLS connected with no client identity. Add an explicit TLSClientCertConfigured marker (robust against nil-vs-empty), set it whenever the option is invoked, and tighten validateKernelConfig to reject any incomplete mTLS request (missing cert, missing key, or both empty). Covered by new option/validation/DeepCopy tests and the exhaustiveness guard. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The ABI-version handshake was only exercised on the happy path (TestABIVersionMatches, cgo build). The mismatch branch — the runtime hazard the check exists for, a driver header linked against a differently-built prebuilt .a — had no coverage because it lived behind the cgo symbols and the one-shot abiCheckOnce cache. Extract the pure got-vs-want verdict into compareABI (untagged) and have checkABIVersion delegate to it, then add TestCompareABI asserting the mismatch returns a non-nil error naming both versions and the remediation. Being untagged, the negative test runs under the default CGO_ENABLED=0 build with no kernel lib linked. Production behavior is unchanged. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Register a cgo-exported trampoline (kernelLogTrampoline) as the kernel's log sink (kernel_set_log_callback) so kernel-internal tracing events flow into the driver's logger — the unified Go+kernel stream, honouring logger.SetLogOutput — instead of only reaching stderr via kernel_init_logging. This completes the one-knob logging story: #399 unified the LEVEL across the Go binding lines and the kernel's Rust lines (PECOBLR-3650) against the stderr sink; this replaces that sink with the callback so the Rust lines land in the same writer as everything else. The reverse-call machinery: a runtime/cgo.Handle round-trips the *logSink through the C void* ctx (cgo pointer rules), and a defer recover() firewall converts any panic into a dropped line (a panic across the cgo boundary would abort the process) — the same shape a kernel->host token-provider callback would need. The driver maps its own log level (resolveKernelLogArg, unchanged) and passes it as the callback's level argument, so the kernel's Rust lines follow DATABRICKS_LOG_LEVEL, not just RUST_LOG; DBSQL_KERNEL_DEBUG still defers to RUST_LOG (NULL level). The forwarding sink logs through a SNAPSHOT of the driver logger taken at install and pinned to TraceLevel, for two reasons: (1) no double gating — the kernel already filtered events against the level we passed it, so re-gating through the live logger.Logger (at the driver level) would re-drop exactly the events the DBSQL_KERNEL_DEBUG override was meant to surface; (2) no data race — the kernel drain thread reads only its immutable snapshot rather than the mutable global logger a concurrent SetLogLevel/SetLogOutput would rewrite. A non-Success install is surfaced at Warn (visible at the default level), not the Debug-gated klog. The two C-ABI logging routes share the one process-global subscriber and are mutually exclusive; we install the callback, and a non-Success install (host already installed a subscriber) is logged, never fatal to connect. Level mapping and sink routing are pure Go (logforward.go, untagged) so they test under CGO_ENABLED=0 — including the per-level mapping assertions and the not-re-gated-by-driver-level property. The reverse-call round-trip, the recover firewall (a panicking sink must not crash the process), the wrong-type/nil-ctx guards, and delivery after a panic are exercised by tagged tests via a C invoke seam. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Remove ticket codes, review shorthand, and ops jargon so the PR reads cleanly for OSS reviewers. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Rely on resolveKernelLogArg returning an empty level for the RUST_LOG override instead of normalizing it twice. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The consolidated branch now carries the log-callback forwarding (log_callback.go references kernel_set_log_callback / kernel_log_callback), so KERNEL_REV must point at a kernel revision that exports those symbols. Bump from the tier2 rev 02c0a434 (pre-log-callback) to 946c263, the head of kernel PR #169 (mani/c-abi-log-callback), which defines them. The kernel-lib build resolves this unmerged SHA via its PR-head-ref fallback, so the tagged kernel-backend build links a header carrying the symbols. Temporary: re-pin to the squash-merge SHA once kernel #169 lands. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Code Review Squad — ReviewScore: 69/100 — HIGH RISK The C-ABI/FFI mechanics are sound — security and language reviewers independently verified cert handling, memory ownership, cancel-token lifecycle, and the log trampoline against the kernel header at the pinned rev, both clean. Risk is concentrated in two test-coverage gaps (a dropped forwarding line or a broken connect-cancel path would ship green) and a UX/contract inconsistency where the standard WithCloudFetch(false) is silently ignored on the kernel path (flagged by 3 reviewers). No confirmed production correctness bug; the HIGH RISK band is driven by the test gaps, not by shipped defects. Medium FindingsM2 — This PR adds mid-fetch cancellation:
This finding has no in-diff line to anchor an inline comment (the paragraph sits just past the last doc.go hunk, which ends at line 266).
|
Test coverage: - Assert buildKernelConfig forwards TLSClientCertPEM / TLSClientKeyPEM / CloudFetchEnabled (previously only trusted-certs + hostname-skip were asserted, so a dropped forwarding line could ship green), plus an unset-CloudFetchEnabled-stays-nil case. - Add a tagged, hermetic mid-connect cancel test: a black-hole TLS listener makes OpenSession block, and a short ctx deadline must fire the cancel token mid-connect and surface as "session_open cancelled" matching DeadlineExceeded. The prior connect-cancel e2e only hit the pre-connect guard; its docstring is corrected to say so. Contract: - Reject a bare WithCloudFetch(false) on the kernel path (steering to WithKernelCloudFetch(false)) instead of silently dropping it and leaving CloudFetch on; classify UseCloudFetch as rejected. When WithKernelCloudFetch is set it stays authoritative. - doc.go: mid-fetch cancellation is now supported (not a limitation); refresh the "nothing silently ignored" list. Operational: - Forward kernel ERROR/WARN one severity lower by default (->Warn/Info, tagged with the native level) so kernel-internal retries don't inflate the driver's alert-keyed WARN/ERROR rate; DBSQL_KERNEL_DEBUG preserves native severity. Cleanup: - Extract cancelledErr() for the shared dual-%w cancelled-path wrap used by OpenSession, execute, and nextBatch. - Create one ctxWatcher per result set (in newKernelRows, stopped in Close) instead of one per Arrow batch, matching the execute path's amortization. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
c111977 to
4137103
Compare
…erage Add a tagged, warehouse-free test that exercises the experimental kernel TLS options end-to-end through the real cgo -> kernel -> rustls stack against a local httptest TLS server: - WithKernelClientCertificate: with a client cert the mTLS handshake completes (handler reached, peer cert observed); against a RequireAndVerifyClientCert server with no cert, the handshake is rejected before the handler runs. - WithKernelTrustedCerts: a privately-signed server cert validates only when its CA is trusted; an untrusted CA fails the chain. - WithKernelSkipHostnameVerify: a wrong-SAN server cert is rejected on hostname grounds unless the skip is set. Certs are minted at runtime with crypto/x509 (PKCS#8 keys, the form the kernel's tls-rustls Identity::from_pem accepts) — no committed fixtures, no external tools. The kernel speaks SEA so the query itself fails at the app layer; the property under test is that the handshake completes with the forwarded material, observed via a per-connection reached/saw-cert probe. Each case cancels the connect the instant the outcome is decided rather than riding the kernel's connect-retry budget, so the file runs in a few seconds. Runs in the tagged build-and-test-kernel job (no warehouse needed) — closing the mTLS/custom-CA gap that had only byte-marshalling unit coverage before. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
|
Re M2 (from the review summary — All 6 inline findings (#1–#6) are fixed and their threads resolved; details in each thread. Head is now |
|
Go integration tests triggered ( |
Follow-ups on the ctx-cancel backend, all behind the cgo && databricks_kernel tag. No behavior change on the default CGO_ENABLED=0 Thrift build. - OpenSession / nextBatch: emit a debug-gated klogCtx line when a connect or mid-fetch is interrupted by the caller's context, right before returning the cancelled error, so a ctx-driven cancel is visible in kernel debug logs (zero cost when kernel debug logging is off). - cancel.go: reword the ctxWatcher doc to match kernel behavior — firing the token is a real abort on connect (request awaited inline) but a prompt stop-waiting on mid-fetch (the CloudFetch download runs on a detached kernel task and drains to its ~60s read-timeout); either way the caller's goroutine is freed at the deadline. Mirrors the reworded kernel header contract. - abi.go: fix a stale comment — checkABIVersion runs abiVersions() once per process (sync.Once-cached), not on every connect. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
|
Integration test approval reset. New commits were pushed to this PR. Label(s) A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.) Latest commit: 84f2b5c |
Point KERNEL_REV at c19e5d1, the current head of kernel PR #169. The prior pin (946c263) was an orphaned SHA from an earlier force-push of the same PR branch. ABI version is unchanged (still 1 — the earlier bump was reverted), and all driver-used C-ABI symbols are present in the header at the new rev. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
dffff64 to
6c77786
Compare
|
Go integration tests triggered ( |
Forward a backend-neutral WithCloudFetch(false) to the kernel CloudFetch toggle instead of rejecting it at connect. UseCloudFetch defaults to true, so false is an unambiguous explicit disable — honoring it (equivalent to WithKernelCloudFetch(false)) is friendlier than the previous hard error and still satisfies the "nothing silently ignored" contract. An explicit WithKernelCloudFetch stays authoritative when both are set. This unblocks the driver-test Go integration suite, whose result-fetching tests disable CloudFetch via the backend-neutral cfg.UseCloudFetch on the SEA leg (TestLazyFetchEmptyResultSet). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
|
Integration test approval reset. New commits were pushed to this PR. Label(s) A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.) Latest commit: 15d276b |
…ath" This reverts commit 15d276b. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
bbb2f00 to
47e7adf
Compare
|
Go integration tests triggered ( |
Match the slimmed kernel PR (#169 → mTLS + ABI-version only): remove the Go wiring for the C-ABI symbols dropped kernel-side, re-pin KERNEL_REV to the updated kernel head (12919b3), and keep the two survivors. Keep: - WithKernelClientCertificate → set_tls_client_certificate (mTLS); the option, config plumbing, unit tests, and kernel_mtls_test.go hermetic handshake test all stay. - checkABIVersion() / abiVersions() at connect (abi.go + abi_test.go + TestABIVersionMatches) — the drift handshake. Remove (no merged consumer; kernel symbols gone): - ctx-cancel bridge: cancel.go (ctxWatcher/cancelledErr), cancel_test.go, cancel_connect_test.go; revert OpenSession to plain kernel_session_open, rows.go nextBatch to plain kernel_result_stream_next_batch, and operation.go's execute-path wrap to its inline form. - log-callback bridge: log_callback.go, logforward.go, logforward_test.go, and the TestLogCallback* cases; initKernelLogging reverts to the kernel_init_logging (stderr) path. - CloudFetch toggle: WithKernelCloudFetch + the CloudFetchEnabled field and its validation/forwarding/classification (UseCloudFetch back to inert). - close_blocking references (never wired; CloseSession stays fire-and-forget). Reverted kernel_e2e_test.go to drop the three cancel-family e2e tests (mid-fetch / query-cancel-during-execution / connect-honors-cancelled-ctx); the pre-existing TestKernelE2ECancellation still covers query cancel. Validated against the new kernel lib: untagged build/vet/tests green; tagged build + vet + kernel-backend unit tests green; live e2e against a warehouse green (Select1, DataTypes ×18, CloudFetch, Cancellation, M2M, TLSSkipVerify, Interval, MetricViewMetadata — all PASS). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
|
Integration test approval reset. New commits were pushed to this PR. Label(s) A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.) Latest commit: bc63801 |
What
Two experimental, kernel-only capabilities on the SEA-via-kernel Go backend,
matching the slimmed kernel PR (databricks-sql-kernel#169):
mTLS client certificate
WithKernelClientCertificate(cert, key)— configure a client certificate +private key for mutual TLS. Both PEM halves are required together (they map to
the single paired
kernel_session_config_set_tls_client_certificate, socert-without-key is unrepresentable); the private key is never logged. An
incomplete request (cert-without-key, key-without-cert, or the option called
with empty bytes from a failed PEM load) is rejected loudly at connect rather
than silently connecting with no client identity. EXPERIMENTAL, kernel-only:
the default (Thrift) backend rejects it at connect with
ErrRequiresKernelBackend.tls_client_cert), Node (napiclient_cert_pem), and JDBC(
SSLKeyStore) already expose the equivalent — this brings the Go/kernel pathto parity.
ABI-version handshake
checkABIVersion()runs once atOpenSession, before any other kernel call:it compares the linked kernel library's
kernel_abi_version()against theDATABRICKS_KERNEL_ABI_VERSIONthe driver's header declares and refuses toopen on a mismatch, guarding against a driver built against one header linked
against a differently-built prebuilt
.a.KERNEL_REVis pinned to the kernel head carrying these two symbols.Scope note
This PR was scoped down during review (thanks @vikrantpuppala on the kernel
side). Earlier revisions also wired ctx-deadline cancellation, a CloudFetch
toggle, and reverse-call log forwarding. Those kernel C-ABI symbols were removed
and deferred (they were purely additive with no merged consumer), so the Go
wiring is removed here to match:
ctxWatcher)— Gap 6/7 (PECOBLR-3645 / PECOBLR-3646).
OpenSession/nextBatchrevert tothe plain (non-cancellable) kernel calls; query cancellation still works via
the pre-existing statement canceller. Re-add when the deadline-honoring path is
actually needed (CloudFetch downloads already carry their own ~60s read-timeout).
WithKernelCloudFetch) — the server gatescan_cloud_download(CONFIG_NOT_AVAILABLE/42K0I), so there's no workinge2e path until SEA inline-Arrow ships server-side. (CloudFetch itself is
unaffected — only the disable knob is dropped.)
kernel_set_log_callbackbridge) —initKernelLoggingreverts tokernel_init_logging(kernel Rust logs tostderr / a file), the pre-existing path; the unified-sink forwarding was an
ergonomics upgrade, not required.
How we know it works
CGO_ENABLED=0): build + vet +go testgreen — the configwiring, experimental-field classification guard, and DeepCopy.
cgo && databricks_kernel, against the pinned kernel.a): build +vet + kernel-backend unit tests green —
TestSetKernelTLS(incl. the mTLScase),
TestABIVersionMatches, execute/error contracts.decimal/variant/geometry), CloudFetch, Cancellation (query cancel still
works), M2M, TLSSkipVerify, Interval, MetricViewMetadata — all PASS.
mTLS is implemented and wired end-to-end in the kernel (verified against an
openssl s_server -Verifyclient-cert-required server: with the identity thehandshake completes; without it the server rejects with
certificate required).No live mTLS e2e here — no Databricks endpoint demands a client cert — so the
kernel_mtls_test.gocoverage is option-wiring + the kernel-side handshake proof.Co-authored-by: Isaac